feat: allow externally managed webhook signing capabilities#876
Merged
Conversation
There was a problem hiding this comment.
LGTM. Right shape: the flag is SDK-internal, never projects to the wire, and the validator's short-circuit is correctly narrow (sender is None AND supervisor is None) so wiring an SDK sender still triggers full RFC 9421 introspection.
Things I checked
webhook_signing_managed_externallylives at the top level ofDecisioningCapabilities(platform.py:181), not inside theWebhookSigningPydantic model. The projection at handler.py:1697-1700 only dumpscaps.webhook_signing.model_dump(...)— flag has zero wire reachability. Pinned by the new projection test at test_decisioning_capabilities_projection.py:360-365.- Bypass safety:
isinstance(adopter_managed, bool)at webhook_emit.py:447 fail-closes on truthy strings, ints,numpy.bool_, etc. The subsequentadopter_managed is Trueat L462 is defense-in-depth — identity, not equality, so1 == Truedoesn't sneak through. - Wired-sender precedence: short-circuit at L462 requires
sender is None and supervisor is None. A bearer-token sender wired alongsidewebhook_signing_managed_externally=Truestill trips the non-JWK gate at L511 — pinned bytest_adopter_managed_flag_does_not_bypass_wired_sender_validation. - Request-scoped path: handler.py:1624 re-runs the validator per-request when
get_adcp_capabilities_for_requestreturns scoped caps, so per-tenant flag flipping works as documented._validate_scoped_capabilities_static_contractconstrains onlyspecialismsandsupported_protocols, leaving the new flag free to vary per-tenant (intended). - Conventional commit
feat:is correct. New field is keyword-defaulted (bool = False); no public-API break, no!required. RequestContext.metadata/_build_request_context/ credential-shaped key suffix list untouched. No credential-leak surface change.- Spec compliance: AdCP
webhook_signing.supported=truebinds the seller to RFC 9421, not the SDK specifically. Moving the contract from SDK-validated to operator-asserted is spec-legal.
Follow-ups (non-blocking — file as issues)
- INFO vs WARNING on the bypass log. webhook_emit.py:463 emits at INFO, but every sibling skip-path in this file uses WARNING (L170, L253, L279, L295, L321, L480) — including the structurally identical "we skipped validation, you own the contract" branch at L480. A fat-fingered
webhook_signing_managed_externally=Trueon a broken external signer should be the loudest line in boot logs, not the quietest. (security-reviewer-deep: Low.) - Validator docstring drift. webhook_emit.py:399-437 still describes unconditional fail-fast; the new bypass lives only in the body. Add a sentence naming the
webhook_signing_managed_externallycarve-out so a reader auditing the contract via the docstring sees the operator-owned escape hatch. (ad-tech-protocol-expert-deep.) - Test gap on the supervisor path. New tests cover sender=None+supervisor=None (passes) and sender=bearer+supervisor=None (still fails). Missing:
supervisor=InMemoryWebhookDeliverySupervisor(bearer_sender) + sender=None + adopter_managed=True— should still fail, since the SDK is doing the signing. The behavior is correct (the supervisor path at L471-L489 doesn't re-consult the flag); the test is the gap. - Optional: scoped-caps gate symmetry.
_validate_scoped_capabilities_static_contract(handler.py:976) blocks scoped caps from changingspecialisms/supported_protocols. Consider extending to reject scoped caps enablingwebhook_signing.supported=Truewhen static caps had it disabled — would close the per-tenant trust-surface expansion. Not a security hole today (failure is loud on the buyer side, contained to the asserting tenant), but symmetric with the existing guard.
Approving on the strength of the fail-closed non-bool guard plus the narrow short-circuit condition.
This was referenced May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DecisioningCapabilities.webhook_signing_managed_externallyfor adopters that sign webhooks outside the SDK sender stackFixes #875
Validation
PYTHONPATH=src python3 -m pytest tests/test_tools_list_output_schema.py tests/test_schema_validation_server.py tests/test_dispatcher_version_routing.py tests/test_decisioning_capabilities_submodule.py tests/test_decisioning_capabilities_projection.py tests/test_webhook_signing_capabilities.py -qPYTHONPATH=src python3 -m pytest tests/test_decisioning_capabilities_projection.py tests/test_webhook_signing_capabilities.py -qPYTHONPATH=src mypy src/adcp/decisioning/platform.py src/adcp/decisioning/webhook_emit.pyruff check src/adcp/decisioning/platform.py src/adcp/decisioning/webhook_emit.py tests/test_webhook_signing_capabilities.py tests/test_decisioning_capabilities_projection.py